Add command-line shard selection and Windows Jump List support#535
Merged
Add command-line shard selection and Windows Jump List support#535
Conversation
Contributor
WalkthroughAdds command-line parsing with a new CommandLineOptions type exposed via App.CurrentOptions, persists a LastPlayed timestamp on shard entries, and updates the Windows JumpList from recently-played shards. Also updates NuGet deps and adds a new localized "Shard not found" resource. Changes
Sequence DiagramsequenceDiagram
participant Launcher as Launcher App
participant Parser as CommandLineParser
participant MainVM as MainViewModel
participant ShardStore as ShardEntry Store
participant JumpList as Windows JumpList
Launcher->>Parser: Parse process arguments
Parser-->>Launcher: Return parsed CommandLineOptions
Launcher->>MainVM: Initialize with CurrentOptions
MainVM->>ShardStore: Load shards & presets (deserialize including LastPlayed)
alt CurrentOptions.Shard specified
MainVM->>ShardStore: Find matching shard
MainVM->>ShardStore: Set SelectedShard.LastPlayed = Now
MainVM->>Launcher: Attempt auto-start / show error if not found
end
MainVM->>ShardStore: Query shards with LastPlayed
ShardStore-->>MainVM: Return sorted recent shards
MainVM->>JumpList: Populate JumpList entries from recent shards
JumpList-->>MainVM: JumpList updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip You can customize the high-level summary generated by CodeRabbit.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ClassicAssist.Launcher/MainViewModel.cs (1)
657-669:⚠️ Potential issue | 🟠 MajorLastPlayed not serialised for non-preset shards.
LastPlayedis read during deserialisation for both presets and user shards (lines 136, 155), and written for presets (line 715), but it's not written for non-preset shards here. User shards will lose theirLastPlayeddata on restart.🐛 Proposed fix to include LastPlayed in shard serialisation
foreach ( ShardEntry shard in shards ) { JObject shardObj = new JObject { { "Name", shard.Name }, { "Address", shard.Address }, { "Port", shard.Port }, { "HasStatusProtocol", shard.HasStatusProtocol }, - { "Encryption", shard.Encryption } + { "Encryption", shard.Encryption }, + { "LastPlayed", shard.LastPlayed } }; shardArray.Add( shardObj ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 657 - 669, The loop that serializes non-preset ShardEntry objects into shardArray is missing the LastPlayed field, causing user shards to lose that value; update the JObject creation inside the foreach (ShardEntry shard in shards) to include { "LastPlayed", shard.LastPlayed } (matching how presets are serialized) so LastPlayed is preserved for non-preset shards; locate the serialization block where shardObj is built and add the LastPlayed property to the object.
🧹 Nitpick comments (1)
ClassicAssist.Launcher/CommandLineOptions.cs (1)
5-9: Consider adding HelpText for better user experience.Adding a
HelpTextparameter to the[Option]attribute would provide users with guidance when they use--help.✨ Suggested improvement
public class CommandLineOptions { - [Option( "shard", Required = false )] + [Option( "shard", Required = false, HelpText = "Name of the shard to launch directly." )] public string Shard { get; set; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/CommandLineOptions.cs` around lines 5 - 9, The CommandLineOptions class's Shard property is missing a user-facing description; update the [Option] attribute on the Shard property (in class CommandLineOptions) to include a HelpText parameter that briefly explains what the --shard option does and any expected value format so it appears in the auto-generated help output for the CLI.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ClassicAssist.Launcher/App.xaml.cs`:
- Around line 10-12: The file's license header in App.xaml.cs contains a
truncated warranty disclaimer; replace the truncated lines at the top with the
full, standard GPL v3 warranty disclaimer paragraph (the "NO WARRANTY"
paragraph) so the license block is complete and accurate, ensuring the header
associated with the App class / App.xaml.cs contains the full GPL v3 disclaimer
text exactly as specified by the license.
- Around line 27-34: CurrentOptions can be left null if argument parsing fails
in OnStartup because
Parser.Default.ParseArguments<CommandLineOptions>(e.Args).WithParsed only sets
CurrentOptions on success; update OnStartup to initialize App.CurrentOptions to
a sensible default CommandLineOptions instance before parsing and handle parse
failures by using WithNotParsed (or checking the result) to log or fall back to
defaults so MainViewModel's access to App.CurrentOptions.Shard is safe;
specifically modify the OnStartup method to set CurrentOptions = new
CommandLineOptions(...) first, then call
Parser.Default.ParseArguments<CommandLineOptions>(e.Args) and use WithParsed to
overwrite CurrentOptions on success and WithNotParsed to keep defaults or handle
errors.
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 10-12: The file header currently contains an incomplete/truncated
license disclaimer; update the top-of-file comment above the MainViewModel class
to include the full standard license disclaimer used across the project (replace
the two truncated lines with the complete multi-line warranty and license text
matching other files). Ensure the complete disclaimer mirrors the exact wording
and formatting used in peer files so the MainViewModel.cs header is consistent
with the project's license headers.
- Around line 203-219: Guard against App.CurrentOptions being null before
accessing App.CurrentOptions.Shard and remove the unreachable redundant null
check: check App.CurrentOptions != null and
!string.IsNullOrEmpty(App.CurrentOptions.Shard) before querying
ShardManager.VisibleShards, find the ShardEntry via
ShardManager.VisibleShards.FirstOrDefault(...), if shard is null show the
MessageBox and return, otherwise assign SelectedShard = shard and then call
StartCommand.Execute(null); remove the extra "if (shard != null)" branch since
the null case already returns.
- Line 562: The code builds playedShards from ShardManager.VisibleShards but
uses OrderBy(e => e.LastPlayed) which sorts ascending so oldest/never-played
appear first; change the sort to OrderByDescending(e => e.LastPlayed) (keeping
the existing Where(e => e.LastPlayed != default)) so ShardEntry items with the
most recent LastPlayed appear first.
In `@ClassicAssist.Launcher/ShardEntry.cs`:
- Around line 10-12: The license disclaimer in ShardEntry.cs is truncated;
update the comment block to include the full, matching warranty disclaimer used
in App.xaml.cs so the header is complete and consistent. Locate the top-of-file
comment in ShardEntry.cs and replace the truncated lines with the full standard
disclaimer text (the same phrasing used in App.xaml.cs) ensuring punctuation and
line breaks match exactly for consistency across files.
---
Outside diff comments:
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 657-669: The loop that serializes non-preset ShardEntry objects
into shardArray is missing the LastPlayed field, causing user shards to lose
that value; update the JObject creation inside the foreach (ShardEntry shard in
shards) to include { "LastPlayed", shard.LastPlayed } (matching how presets are
serialized) so LastPlayed is preserved for non-preset shards; locate the
serialization block where shardObj is built and add the LastPlayed property to
the object.
---
Nitpick comments:
In `@ClassicAssist.Launcher/CommandLineOptions.cs`:
- Around line 5-9: The CommandLineOptions class's Shard property is missing a
user-facing description; update the [Option] attribute on the Shard property (in
class CommandLineOptions) to include a HelpText parameter that briefly explains
what the --shard option does and any expected value format so it appears in the
auto-generated help output for the CLI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 31524350-1f26-472c-aa08-5071ee4f6f2a
📒 Files selected for processing (6)
ClassicAssist.Launcher/App.xaml.csClassicAssist.Launcher/ClassicAssist.Launcher.csprojClassicAssist.Launcher/CommandLineOptions.csClassicAssist.Launcher/MainViewModel.csClassicAssist.Launcher/ShardEntry.csClassicAssist.Launcher/Utility.cs
Comment on lines
+203
to
+219
| if ( !string.IsNullOrEmpty( App.CurrentOptions.Shard ) ) | ||
| { | ||
| ShardEntry shard = ShardManager.VisibleShards.FirstOrDefault( e => e.Name == App.CurrentOptions.Shard ); | ||
|
|
||
| if ( shard == null ) | ||
| { | ||
| MessageBox.Show( @"Shard not found", Strings.Error ); | ||
| return; | ||
| } | ||
|
|
||
| if ( shard != null ) | ||
| { | ||
| SelectedShard = shard; | ||
| } | ||
|
|
||
| StartCommand.Execute( null ); | ||
| } |
Contributor
There was a problem hiding this comment.
Redundant null check and potential NullReferenceException.
Two issues here:
-
NullReferenceException:
App.CurrentOptionsmay be null if command-line parsing fails (see App.xaml.cs review). This access at line 203 would throw. -
Redundant code: The
if ( shard != null )check at line 213 is unreachable because ifshard == null, the code returns at line 210.
🐛 Proposed fix
- if ( !string.IsNullOrEmpty( App.CurrentOptions.Shard ) )
+ if ( App.CurrentOptions != null && !string.IsNullOrEmpty( App.CurrentOptions.Shard ) )
{
ShardEntry shard = ShardManager.VisibleShards.FirstOrDefault( e => e.Name == App.CurrentOptions.Shard );
if ( shard == null )
{
MessageBox.Show( @"Shard not found", Strings.Error );
return;
}
- if ( shard != null )
- {
- SelectedShard = shard;
- }
+ SelectedShard = shard;
StartCommand.Execute( null );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if ( !string.IsNullOrEmpty( App.CurrentOptions.Shard ) ) | |
| { | |
| ShardEntry shard = ShardManager.VisibleShards.FirstOrDefault( e => e.Name == App.CurrentOptions.Shard ); | |
| if ( shard == null ) | |
| { | |
| MessageBox.Show( @"Shard not found", Strings.Error ); | |
| return; | |
| } | |
| if ( shard != null ) | |
| { | |
| SelectedShard = shard; | |
| } | |
| StartCommand.Execute( null ); | |
| } | |
| if ( App.CurrentOptions != null && !string.IsNullOrEmpty( App.CurrentOptions.Shard ) ) | |
| { | |
| ShardEntry shard = ShardManager.VisibleShards.FirstOrDefault( e => e.Name == App.CurrentOptions.Shard ); | |
| if ( shard == null ) | |
| { | |
| MessageBox.Show( @"Shard not found", Strings.Error ); | |
| return; | |
| } | |
| SelectedShard = shard; | |
| StartCommand.Execute( null ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 203 - 219, Guard
against App.CurrentOptions being null before accessing App.CurrentOptions.Shard
and remove the unreachable redundant null check: check App.CurrentOptions !=
null and !string.IsNullOrEmpty(App.CurrentOptions.Shard) before querying
ShardManager.VisibleShards, find the ShardEntry via
ShardManager.VisibleShards.FirstOrDefault(...), if shard is null show the
MessageBox and return, otherwise assign SelectedShard = shard and then call
StartCommand.Execute(null); remove the extra "if (shard != null)" branch since
the null case already returns.
|
|
||
| JumpList jumpList = new JumpList { ShowRecentCategory = true }; | ||
|
|
||
| IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderBy( e => e.LastPlayed ); |
Contributor
There was a problem hiding this comment.
OrderBy sorts ascending — most recent shards will appear last.
For a "recently played" list, users typically expect the most recently played items first. OrderBy sorts ascending, so DateTime.MinValue (never played) appears first and the most recent appears last.
🔧 Proposed fix
- IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderBy( e => e.LastPlayed );
+ IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderByDescending( e => e.LastPlayed );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist.Launcher/MainViewModel.cs` at line 562, The code builds
playedShards from ShardManager.VisibleShards but uses OrderBy(e => e.LastPlayed)
which sorts ascending so oldest/never-played appear first; change the sort to
OrderByDescending(e => e.LastPlayed) (keeping the existing Where(e =>
e.LastPlayed != default)) so ShardEntry items with the most recent LastPlayed
appear first.
Allows launching specific shards directly using a new `--shard` command-line argument. The launcher now tracks the last played date for shards and populates the Windows Taskbar Jump List with recently played entries for quick access. This change also updates several dependencies and adds license headers to modified files.
Reetus
force-pushed
the
launcher-jumplist
branch
from
e072f77 to
fda5b93
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ClassicAssist.Launcher/MainViewModel.cs (1)
652-664:⚠️ Potential issue | 🟠 Major
LastPlayedis not persisted for user-added (non-preset) shards.When serialising user shards (lines 652-664),
LastPlayedis not included in the output. However, when deserialising (lines 159-160),LastPlayedis read. This means user shards will lose theirLastPlayedtimestamps between sessions, and they won't appear in the JumpList after restarting the application.For consistency with the Presets serialisation (lines 709-710), add
LastPlayedto the user shards output.🐛 Proposed fix
JObject shardObj = new JObject { { "Name", shard.Name }, { "Address", shard.Address }, { "Port", shard.Port }, { "HasStatusProtocol", shard.HasStatusProtocol }, - { "Encryption", shard.Encryption } + { "Encryption", shard.Encryption }, + { "LastPlayed", shard.LastPlayed } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 652 - 664, The user-added ShardEntry objects are being serialized without the LastPlayed field, causing LastPlayed to be lost despite MainViewModel deserializing it later; update the serialization block inside the shards foreach (the creation of shardObj added to shardArray) to include the "LastPlayed" property from shard.LastPlayed (matching the Presets serialization format) so that shardArray contains { "LastPlayed", shard.LastPlayed } alongside Name, Address, Port, HasStatusProtocol and Encryption.
♻️ Duplicate comments (1)
ClassicAssist.Launcher/MainViewModel.cs (1)
557-557:⚠️ Potential issue | 🟡 Minor
OrderBysorts ascending — most recent shards will appear last in the JumpList.For a "recently played" list, users typically expect the most recently played items to appear first.
OrderBysorts ascending, so the oldest played shards appear first and the most recent appears last.🔧 Proposed fix
- IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderBy( e => e.LastPlayed ); + IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderByDescending( e => e.LastPlayed );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/MainViewModel.cs` at line 557, The playedShards sequence is currently ordered ascending by LastPlayed so the newest shards show last; change the LINQ ordering on ShardManager.VisibleShards (where you build IOrderedEnumerable<ShardEntry> playedShards) from OrderBy(e => e.LastPlayed) to OrderByDescending(e => e.LastPlayed) so the most recently played ShardEntry items appear first in the JumpList.
🧹 Nitpick comments (1)
ClassicAssist.Launcher/MainViewModel.cs (1)
543-574: JumpList implementation looks good overall.The
UpdateJumpListmethod correctly:
- Retrieves the current process executable path
- Creates JumpTask entries with proper arguments for the
--shardparameter- Applies the JumpList to the current application
One consideration: if there are many shards with
LastPlayedset, the JumpList could become quite long. Windows typically limits JumpList items, but you may want to consider adding a.Take(10)or similar to limit entries to the most recently played.💡 Optional: Limit JumpList entries
- IOrderedEnumerable<ShardEntry> playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderByDescending( e => e.LastPlayed ); + var playedShards = ShardManager.VisibleShards.Where( e => e.LastPlayed != default ).OrderByDescending( e => e.LastPlayed ).Take( 10 );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ClassicAssist.Launcher/MainViewModel.cs` around lines 543 - 574, UpdateJumpList currently enumerates all played shards (playedShards from ShardManager.VisibleShards) which can produce an oversized JumpList; modify the LINQ used to build playedShards in UpdateJumpList to sort by most recent (use OrderByDescending on ShardEntry.LastPlayed) and then limit the collection (e.g. .Take(10)) before iterating to create JumpTask entries, so only the N most-recent shards are added to the JumpList/JumpTask population.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ClassicAssist.Launcher/CommandLineOptions.cs`:
- Line 8: The Shard property currently stores input verbatim which can cause
mismatches when MainViewModel.cs compares e.Name == App.CurrentOptions.Shard;
update the Shard property in ClassicAssist.Launcher.CommandLineOptions to
normalize input by trimming whitespace (and optionally normalize case, e.g.,
ToLowerInvariant()) in its setter and treat empty/whitespace-only values as null
or empty string so downstream exact equality comparisons won't fail due to
leading/trailing spaces.
---
Outside diff comments:
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 652-664: The user-added ShardEntry objects are being serialized
without the LastPlayed field, causing LastPlayed to be lost despite
MainViewModel deserializing it later; update the serialization block inside the
shards foreach (the creation of shardObj added to shardArray) to include the
"LastPlayed" property from shard.LastPlayed (matching the Presets serialization
format) so that shardArray contains { "LastPlayed", shard.LastPlayed } alongside
Name, Address, Port, HasStatusProtocol and Encryption.
---
Duplicate comments:
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Line 557: The playedShards sequence is currently ordered ascending by
LastPlayed so the newest shards show last; change the LINQ ordering on
ShardManager.VisibleShards (where you build IOrderedEnumerable<ShardEntry>
playedShards) from OrderBy(e => e.LastPlayed) to OrderByDescending(e =>
e.LastPlayed) so the most recently played ShardEntry items appear first in the
JumpList.
---
Nitpick comments:
In `@ClassicAssist.Launcher/MainViewModel.cs`:
- Around line 543-574: UpdateJumpList currently enumerates all played shards
(playedShards from ShardManager.VisibleShards) which can produce an oversized
JumpList; modify the LINQ used to build playedShards in UpdateJumpList to sort
by most recent (use OrderByDescending on ShardEntry.LastPlayed) and then limit
the collection (e.g. .Take(10)) before iterating to create JumpTask entries, so
only the N most-recent shards are added to the JumpList/JumpTask population.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9de12ba9-fdc4-4400-ad71-f69a6f15e630
📒 Files selected for processing (8)
ClassicAssist.Launcher/App.xaml.csClassicAssist.Launcher/ClassicAssist.Launcher.csprojClassicAssist.Launcher/CommandLineOptions.csClassicAssist.Launcher/MainViewModel.csClassicAssist.Launcher/Properties/Resources.Designer.csClassicAssist.Launcher/Properties/Resources.resxClassicAssist.Launcher/ShardEntry.csClassicAssist.Launcher/Utility.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- ClassicAssist.Launcher/Utility.cs
- ClassicAssist.Launcher/App.xaml.cs
- ClassicAssist.Launcher/ClassicAssist.Launcher.csproj
| public class CommandLineOptions | ||
| { | ||
| [Option( "shard", Required = false )] | ||
| public string Shard { get; set; } |
Contributor
There was a problem hiding this comment.
Normalise Shard input to avoid false “not found” matches.
On Line 8, Shard is stored as-is, but downstream matching uses exact equality (e.Name == App.CurrentOptions.Shard in ClassicAssist.Launcher/MainViewModel.cs). A trailing/leading space in CLI input will fail matching unnecessarily.
Proposed fix
public class CommandLineOptions
{
+ private string _shard = string.Empty;
+
[Option( "shard", Required = false )]
- public string Shard { get; set; }
+ public string Shard
+ {
+ get => _shard;
+ set => _shard = value?.Trim() ?? string.Empty;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ClassicAssist.Launcher/CommandLineOptions.cs` at line 8, The Shard property
currently stores input verbatim which can cause mismatches when MainViewModel.cs
compares e.Name == App.CurrentOptions.Shard; update the Shard property in
ClassicAssist.Launcher.CommandLineOptions to normalize input by trimming
whitespace (and optionally normalize case, e.g., ToLowerInvariant()) in its
setter and treat empty/whitespace-only values as null or empty string so
downstream exact equality comparisons won't fail due to leading/trailing spaces.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allows launching specific shards directly using a new
--shardcommand-line argument. The launcher now tracks the last played date for shards and populates the Windows Taskbar Jump List with recently played entries for quick access.This change also updates several dependencies and adds license headers to modified files.
Summary by CodeRabbit
New Features
Chores